Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix scrolling to the end within a popup #10181

Merged
merged 1 commit into from
Apr 8, 2024

Conversation

karthago1
Copy link
Contributor

when the available height for the popup is low/small, then it is not possible to scroll until the end

this recording shows the problem (created by @thomasaarholt)

when the available height for the popup is low/small, then it is not
possible to scroll until the end

Signed-off-by: Ben Fekih, Hichem <[email protected]>
Copy link
Member

@pascalkuthe pascalkuthe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch finding this! To be honest I don't like the retroactive patching going on here.

I think this may ultimately be hinting at some design issues with the component API and much larger issues with PopUp (there is for example duplication between area and size, and mutating self in required_size is a huge hack to begin with).

I think this makes sense to fix this way for now but we need to seriously reconsider component/popup in the longterm here. Its turning into a mess

@karthago1
Copy link
Contributor Author

I agree, this is not the right way to fix this issue. The mutation inside required_size is a bad thing. I will try to improve the code and maybe I will create another PR

@kirawi kirawi added C-bug Category: This is a bug A-helix-term Area: Helix term improvements S-waiting-on-review Status: Awaiting review from a maintainer. labels Apr 7, 2024
@the-mikedavis the-mikedavis changed the title bugfix: cannot scroll until the end inside a popup Fix scrolling to the end within a popup Apr 8, 2024
@the-mikedavis the-mikedavis merged commit ea2a485 into helix-editor:master Apr 8, 2024
6 checks passed
postsolar pushed a commit to postsolar/helix that referenced this pull request Apr 20, 2024
when the available height for the popup is low/small, then it is not
possible to scroll until the end

Signed-off-by: Ben Fekih, Hichem <[email protected]>
mtoohey31 pushed a commit to mtoohey31/helix that referenced this pull request Jun 2, 2024
when the available height for the popup is low/small, then it is not
possible to scroll until the end

Signed-off-by: Ben Fekih, Hichem <[email protected]>
Vulpesx pushed a commit to Vulpesx/helix that referenced this pull request Jun 7, 2024
when the available height for the popup is low/small, then it is not
possible to scroll until the end

Signed-off-by: Ben Fekih, Hichem <[email protected]>
smortime pushed a commit to smortime/helix that referenced this pull request Jul 10, 2024
when the available height for the popup is low/small, then it is not
possible to scroll until the end

Signed-off-by: Ben Fekih, Hichem <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements C-bug Category: This is a bug S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants